PayjoinSessionFailed is used for infrastructure failures (coin selection, InputPair construction) and network errors (poll/post requests). The upstream lib distinguishes these via SelectionError, InputContributionError, and ReceiverError , would it be worth preserving that granularity so callers have more info and know how to act?

These are the errors exposed at the ldk-node API level. More granular details from the upstream errors are logged for debugging purposes.

since testmempoolaccept is local validation only, shouldn't this use DEFAULT_TX_LOOKUP_TIMEOUT_SECS?

Yes, will update to use DEFAULT_TX_LOOKUP_TIMEOUT_SECS

matching errors by string seems fragile, as the message varies by server implementation.
e.g. tested: ssl://electrum.blockstream.info:60002: Protocol(String("missing transaction")) , which doesn't match either string here and Ok(None) would never be returned. the caller sees a sync failure instead

this is a single onchain tx lookup, should this use tx_broadcast_timeout_secs (10s) instead of lightning_wallet_sync_timeout_secs (30s)? or would a dedicated tx_lookup_timeout_secs field make more sense?

this seems aggressive for a daemon (payjoin-cli uses 200ms because its a one-shot CLI with manual retry) but for ldk node maybe 1-5 seconds would be more appropriate?

finalize_proposal returns MaybeTransientTransition to distinguish transient from fatal errors, but .map_err(|_| Error::PersistenceFailed) collapses both into the same error. Transient failures won't be retried and will surface as PersistenceFailed, which is misleading

PersistenceFailed here doesn't cancel the session. The session remains active in its current state and will be retried on the next poll cycle. Sessions are only marked as failed when we receive a Closed event. Retrying transient failures is handled naturally by the polling loop.

all other PaymentKind variants use even TLV tags (0, 2, 4, 6, 8, 10), was 11 intentional, or should this be 12?

11 is intentional here. Since this is newly added, we're following the "it's okay to be odd" rule, which allows odd TLV types for optional fields that can be safely ignored by older readers. So 11 is correct rather than 12.

payjoin_session_store_res doesn't handle ErrorKind::NotFound, which will occur on any node that has never used payjoin. fix suggestion in read_payjoin_sessions

Thanks for the suggestion, but the list() implementations across all return Ok(vec![]) when the namespace is empty or hasn't been written to yet.

should this pin a rev?

list() returns NotFound when the namespace has never been written, maybe consider something like this handling it?

rust.or_else(|e| if e.kind() == lightning::io::ErrorKind::NotFound {
    Ok(vec![])
} else {
    Err(e)
})?;

Thanks for the suggestion, but the list() implementations across all return Ok(vec![]) when the namespace is empty or hasn't been written to yet.

test_session_fatal_error covers Failure with fallback_tx: None, when check_broadcast_suitability rejects the PSBT. In that case, the ? returns before failed status is persisted

The broadcast suitability check is important for non-interactive receivers (e.g. payment processors) to protect against probing attacks, where a malicious sender can repeatedly send proposals to have the non-interactive receiver reveal the UTXOs it owns with the proposals it modifies. The "broadcastable" check ensures the receiver can broadcast the fallback transaction in case the payjoin fails, effectively incurring a cost to the attacker.

Electrum's lack of support for this kind of check is a longstanding issue unfortunately.

I'm not familiar enough with LDK-node usage to know if it's typically used in an interactive way (receiver takes manual action to receive payment) or non-interactive (e.g. bolt12 or similar static payment with no receiver action per payment)?

In interactive conditions, the receiver may use the assume_interactive_receiver transition method to explicitly skip the broadcast check altogether. In non-interactive conditions though, we should find an alternative way to perform this check. Or at least return an error instead of silently skipping the check, so it can be handled by the implementer.

So here for example we could call proposal.assume_interactive_receiver() if applicable.

Are these failure modes handled by broadcasting the fallback transaction? I'm trying to find where these errors are handled but not sure I'm tracing it correctly.

We can make this a list of OHTTP relays and randomize the relay per-request. This also improves reliability by enabling retries to other relays in case one is offline for any reason.

It may be desirable to keep "Completed" sessions ~forever since it makes it possible to reconstruct past Payjoins for e.g. displaying transaction history.